Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRC class implementation #5911

Merged
merged 4 commits into from
Feb 28, 2018
Merged

CRC class implementation #5911

merged 4 commits into from
Feb 28, 2018

Conversation

deepikabhavnani
Copy link

@deepikabhavnani deepikabhavnani commented Jan 24, 2018

Alternate CRC implementation to #5669.

CRC class MbedCRC.h is templated class created to support hardware/software CRCs. Default CRC will be hardware CRC when support for HAL is available.

Polynomial tables are available for 8/16 bit CCITT, 7/16 bit for SD card and 32-bit ANSI. Polynomial table implementation will be used if Hardware CRC is not available.

In case device does not have hardware CRC and polynomial table is not supported, CRC is still available and is computed runtime bit by bit for all data input.

Here is the example for new implementation. https://gist.github.com/deepikabhavnani/44737819d512c86de49ceeabf8867ef7

Default constructor without any arguments is valid only for supported CRC polynomials.
MbedCRC<POLY_7BIT_SD, 7> ct; --- Valid
MbedCRC<0x07, 7> ct; --- Valid
MbedCRC<0x07, 7> ct(a,b,c,d) -- Valid - Note: Though inital/final/.... values are different, they do not affect the table hence ROM table will be used.
MbedCRC<0x7, 8> ct; --- Invalid, compilation error
If a user passed non supported polynomial, then it will give compilation error same as we have in call backs.

For non-supported polynomials, user need to provide other constructor args as well.
MbedCRC<0x7, 8> ct(a, b, c, d); -- Valid - Note we do not have ROM table for non-supported polynomials, hence it will be either HW or Bitwise.

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are differences between these 2 implementations ? Why would I choose this one instead of the earlier one? +/- ?

/** \addtogroup drivers */
/** @{*/

extern const uint8_t _Table_CRC_7Bit_SD[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global symbol like this starting with _ ? I would just leave it without

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xc0170 It is prohibited to have an identifier name starting by a single underscore and followed by an uppercase letter. These are reserved for the C and C++ library implementation and compiler intrinsic (_Bool is an example).

#define MBED_CRC_API_H

#include <stdint.h>
#include <TableCRC.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"TableCRC.h"

*/
MbedCRC(uint32_t intial_xor, uint32_t final_xor, bool reflect_data, bool reflect_remainder);
MbedCRC();
virtual ~MbedCRC() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ new line

Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepikabhavnani Good submission, I have few questions:

  • Could you detail how hardware CRC will be supported and how it fit with the MbedCRC template class ?
  • I'm not sure to understand the polymorphic model of MbedCRC, compute is virtual but init, deinit and all the partial computations aren't. Could you explain how MbedCRC should be used in polymorphic context and how it should be subclassed ?
  • The member variables if MbedCRC are set at initialization not mutated afterwards. Is there a reason why these variables are not const (or part of the template list) ?

I've also noticed that most code can be completely extracted out of the class (as plain old functions). It may be interesting to extract it to avoid code bloat.


#if DEVICE_CRC

namespace mbed {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

namespace is not a valid C keyword. Is it a C file or a C++ one ?

/** \addtogroup drivers */
/** @{*/

extern const uint8_t _Table_CRC_7Bit_SD[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xc0170 It is prohibited to have an identifier name starting by a single underscore and followed by an uppercase letter. These are reserved for the C and C++ library implementation and compiler intrinsic (_Bool is an example).


/* Default values for different types of polynomials
*/
template <uint32_t polynomial, uint8_t width> MbedCRC<polynomial, width>::MbedCRC(uint32_t intial_xor, uint32_t final_xor, bool reflect_data, bool reflect_remainder):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you split into two lines the template parameter list and the function signature ?

}

private:
uint8_t _width;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that _polynomial and _width are required ? They are already part of the template definition.

* @param data final crc value
* @return Reflected CRC value
*/
uint32_t reflect_remainder(uint32_t data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can be marked as const.

* @param crc CRC value is filled in, but the value is not the final
* @return 0 on success or a negative error code on failure
*/
int32_t bitwise_compute_partial(void *buffer, crc_data_size_t size, uint32_t *crc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can be marked as const. buffer can be const.

MBED_ASSERT(crc != NULL);
MBED_ASSERT(buffer != NULL);

uint8_t *data = static_cast<uint8_t *>(buffer);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

* @return 0 on success or a negative error code on failure
*/

int32_t table_compute_partial(void *buffer, crc_data_size_t size, uint32_t *crc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can be marked as const. buffer can be const.

*/
template <uint32_t polynomial, uint8_t width> MbedCRC<polynomial, width>::MbedCRC(uint32_t intial_xor, uint32_t final_xor, bool reflect_data, bool reflect_remainder):
_inital_value(intial_xor), _final_xor(final_xor), _reflect_data(reflect_data), _reflect_remainder(reflect_remainder)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you initialize the class member in the constructor initialization list ?

* @param reflect_data
* @param reflect_remainder
*/
MbedCRC(uint32_t intial_xor, uint32_t final_xor, bool reflect_data, bool reflect_remainder);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: intial_xor

@deepikabhavnani
Copy link
Author

What are differences between these 2 implementations ? Why would I choose this one instead of the earlier one? +/- ?

Previous implementation had multiple classes, and support for different CRC techniques, all of which were not required. According to @sg- it will be good to have HW as default and Table based if HW does not support. Bitwise only when HW and ROM table is not present in code. Also, he suggested to have single template class and no inheritance as it was confusing and usage was not that good.

First implementation can support all techniques, but little confusing for user. Second one is user friendly with basic functionality. Adding support of other techniques like HalfByteCRC will be difficult in new implementation, but on other hand HAL / HW support will be easy.

@deepikabhavnani
Copy link
Author

Could you detail how hardware CRC will be supported and how it fit with the MbedCRC template class ?

For hardware crc, HAL should have API to check polynomial supported by specific target. In constructor, same API will be used to check is if polynomial is supported by hardware, and set the private bool member "_isHW" accordingly. In rest of the member functions, decision will be based on private member "isHW" and HAL functions will be added with proper locking.

MbedCRC.c file was added for HAL, it is not required at present (missed to remove it :-( )

@deepikabhavnani
Copy link
Author

I'm not sure to understand the polymorphic model of MbedCRC, compute is virtual but init, deinit and all the partial computations aren't. Could you explain how MbedCRC should be used in polymorphic context and how it should be subclassed ?

Sorry for the confusion, none of the member functions should be virtual (I missed to remove). We will have single class to support CRC and no-inheritance. Hope this answers your query.

@deepikabhavnani
Copy link
Author

The member variables if MbedCRC are set at initialization not mutated afterwards. Is there a reason why these variables are not const (or part of the template list) ?

I am not sure if I understood this question properly, can you help me understand which member variable you are taking about?

@pan-
Copy link
Member

pan- commented Jan 24, 2018

Whoops I should have read my sentences before submitting them.

Member variables of the MbedCRC class are set at construction time and never modified in member functions. Therefore these variable are good candidates for const qualification. By extension it should be possible to transform these variables into template parameters but this may be overkill.

@deepikabhavnani
Copy link
Author

@pan- Agree all variables (passed to constructor) are constant and set once. But using them as part of template will not be user friendly and we will have hell lot of combinations for specialization case where we use ROM tables.

CRC class `MbedCRC.h` is templated class created to support hardware/software
CRCs. Default CRC will be hardware CRC when support for HAL is available.

Polynomial tables are available for 8/16 bit CCITT, 7/16 bit for SD card and
32-bit ANSI. Polynomial table implementation will be used if Hardware CRC is
not available.

In case device does not have hardware CRC and polynomial table is not supported,
CRC is still available and is computed runtime bit by bit for all data input.
*/
MbedCRC(uint32_t initial_xor, uint32_t final_xor, bool reflect_data, bool reflect_remainder);
MbedCRC();
virtual ~MbedCRC()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the virtual specifier if the class is not meant to be used in a polymorphic way.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the driver files have destructor and mutex as virtual functions, was not sure why so tried to be consistent with that. With HW implementation, mutex will be required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification; it makes more sense if the class embeds lock/unlock functions that can be overridden.

*
* @return 0 on success or a negative error code on failure
*/
int32_t init(void)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this will do something when hardware CRC will be available. Is it a correct assumption ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about that, it's an interface, I wouldn't like people to use it:

MbedCRC crc(...);
crc.init();
crc.compute(...);
crc.deinit();

Can't we just use constructor/destructor for (de)init code?

*/
void mbed_crc_ctor(void) const
{
MBED_STATIC_ASSERT(width <= 32, "Max 32-bit CRC supported");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use MBED_STATIC_ASSERT at the class scope. It is not necessary to wrap it into a specific function called in the constructor.

Copy link
Author

@deepikabhavnani deepikabhavnani Jan 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it intentionally over there, so that we have common constructor function, where hardware CRC check and other HW CRC related stuff can be done else I will have to keep empty function. Can we move it when we add support for Hardware?

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Could you share your thoughts on how to integrate HW CRC into this?


/** Lifetime of CRC object
*
* @param initial_xor Inital value/seed to Xor (Default ~0x0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no default parameter for the initial_xor and final_xor so saying here that default is ~0x0 is confusing.

POLY_32BIT_ANSI = 0x04C11DB7, // x32+x26+x23+x22+x16+x12+x11+x10+x8+x7+x5+x4+x2+x+1
} crc_polynomial_t;

/** CRC object provides CRC generation through hardware/software
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get some short example of the usage here for both compute and partial? Also could we get some worlds about algorithm used (ignoring hw for now).

*
* @return 0 on success or a negative error code on failure
*/
int32_t init(void)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about that, it's an interface, I wouldn't like people to use it:

MbedCRC crc(...);
crc.init();
crc.compute(...);
crc.deinit();

Can't we just use constructor/destructor for (de)init code?

*/
int32_t compute(void *buffer, crc_data_size_t size, uint32_t *crc)
{
int32_t status;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we assert on crc being null?

int32_t compute_partial(void *buffer, crc_data_size_t size, uint32_t *crc)
{
if (NULL == _crc_table) {
// Compute Slow CRC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that official name of the algorithm? If not can we not use slow as it sounds bad.


#if defined ( __CC_ARM )
#elif defined ( __GNUC__ )
#pragma GCC diagnostic pop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment about what does it do?

Copy link
Author

@deepikabhavnani deepikabhavnani Feb 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am disabling warnings reported as "Shift count is negative", which is kind of invalid warning.

".\mbed-os\drivers/MbedCRC.h(231): warning: #62-D: shift count is negative
detected during instantiation of "std::int32_t mbed::MbedCRC<polynomial, width>::compute_partial_stop(std::uint32_t *) [with polynomial=79764919U, width=(std::uint8_t)' ']" at line 15 of "main.cpp""

Code

if ((width < 8) && (NULL == _crc_table)) {
            p_crc = (uint32_t)(p_crc << (8 - width));
 }

Compiler warns of the shift operation with width as it is width=(std::uint8_t), but we check for ( width < 8) before performing shift, so it should not be an issue.

Do we add such detailed information in code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you just add one sentence description, it will save combined hours of googling over couple of years ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's switching it off... ignore me

@deepikabhavnani
Copy link
Author

@bulislaw

Looks great! Could you share your thoughts on how to integrate HW CRC into this?

For hardware crc, HAL should have API to check polynomial supported by specific target. In constructor, same API will be used to check is if polynomial is supported by hardware, and set the private bool member "_isHW" accordingly. In rest of the member functions, decision will be based on private member "isHW" and HAL functions will be added with proper locking.

Can't we just use constructor/destructor for (de)init code?

We can use constructor/destructor for software CRC as there is no failure case during initialization. But in case of hardware, initialization in init will help in case of failures and will be consistent with other drivers.

/** \addtogroup drivers */
/** @{*/

const uint8_t Table_CRC_7Bit_SD[] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All definitions should go into a cpp file.

#define MBED_CRC_API_H

#include <stdint.h>
#include "drivers/TableCRC.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mbed_assert.h header is missing.

@bulislaw
Copy link
Member

bulislaw commented Feb 5, 2018

how does this relates to #5669 ? Should we try to unify it (aka close one)?

@deepikabhavnani
Copy link
Author

@bulislaw #5669 is closed, and review comments are addressed.

@sg- Please review

@MikeDK
Copy link
Contributor

MikeDK commented Feb 7, 2018

If I'm allowed to make one comment...: I think the crc table should be #ifdef'ed with a macro which tells the compiler if the current target has hw crc support ... Or else this table will just waste flash space when it is not used due to hw crc capability. (for example add 'CRC' to the 'device_has'-parts in the targets/targets.json file, which will result in a preprocessor macro "DEVICE_CRC")

This should be done as soon as the HAL is specified and implemented.

Template specialized functions are moved to cpp file, to add MbedCRC.h
in mbed.h. Else linker shall multiple definition error.
@deepikabhavnani
Copy link
Author

/morph build

@mbed-ci
Copy link

mbed-ci commented Feb 20, 2018

Build : SUCCESS

Build number : 1191
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5911/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Feb 20, 2018

@mbed-ci
Copy link

mbed-ci commented Feb 20, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 21, 2018

@bulislaw Can you review after changes?

@cmonr cmonr dismissed bulislaw’s stale review February 23, 2018 02:55

Three-week old review is stale, and no updated comments in two weeks. Re-review requested.

@cmonr
Copy link
Contributor

cmonr commented Feb 23, 2018

Holding off on merging this PR to wait for @bulislaw's and @SenRamakri's reviews. Not critical for 5.7.6, but will likely need to be completed by End of Next Week.

bulislaw
bulislaw previously approved these changes Feb 26, 2018
0x220, 0x8225, 0x822f, 0x22a, 0x823b, 0x23e, 0x234, 0x8231, 0x8213, 0x216, 0x21c, 0x8219
};

uint32_t Table_CRC_32bit_ANSI[256] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at these tables in this code file, they need to reside in RAM (we change them - I could find that we use them for calculations but never store anything in these) ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at these tables in this code file, they need to reside in RAM

Tables were moved to ROM section from RAM after discussion with @sg- @pan- and @geky

I could find that we use them for calculations but never store anything in these

Yes, they are read-only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to be stored in ROM I believe they need to also be marked const. The compiler doesn't know if they could be modified otherwise.

#include "drivers/TableCRC.h"
#include "platform/mbed_assert.h"

#if defined ( __CC_ARM )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cant find a reference for these lines here. why push/pop is gere for GCC and that negative shift warning ignored.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am disabling warnings reported as "Shift count is negative", which is kind of invalid warning.
Warning is from below code section.
https://github.com/ARMmbed/mbed-os/pull/5911/files/af0982295f010f3dd45ade36ddc9155a5f86289e#diff-a353a07f9612be80e818a21f61ec8409R210

if ((width < 8) && (NULL == _crc_table)) {
            p_crc = (uint32_t)(p_crc << (8 - width));
 }

Compiler warns of the shift operation with width as it is width=(std::uint8_t), but we check for ( width < 8) before performing shift, so it should not be an issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is useful to know, please leave either in the code short description or commit message.

*/
uint32_t get_crc_mask(void) const
{
return (width < 8 ? ((1u << 8)-1) : (uint32_t)((uint64_t)(1ull << width) - 1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

((1u << 8)-1) spaces around -

@@ -107,7 +107,7 @@ uint16_t Table_CRC_16bit_IBM[256] = {
0x220, 0x8225, 0x822f, 0x22a, 0x823b, 0x23e, 0x234, 0x8231, 0x8213, 0x216, 0x21c, 0x8219
};

uint32_t Table_CRC_32bit_ANSI[256] = {
extern const uint32_t Table_CRC_32bit_ANSI[256] = {
Copy link
Contributor

@0xc0170 0xc0170 Feb 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extern is useless here as it's definition, external linkage by default .

One question related, you have sizes in definition and declaration. You could set these to a macro, or a declaration does not need the size, thus if t his changes, no need to change the declaration.
Declaration can be: extern const uint32_t Table_CRC_32bit_ANSI[];.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

external linkage is default in C, but with C++ if array constant is inside the namespace default linkage is internal.
I will replace 256 to a macro, but will keep the size in declaration as well.

@deepikabhavnani
Copy link
Author

/morph build

@deepikabhavnani
Copy link
Author

@0xc0170 - Please review the changes, and let me know if your comments are not addressed properly

@mbed-ci
Copy link

mbed-ci commented Feb 28, 2018

Build : SUCCESS

Build number : 1296
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5911/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Feb 28, 2018

@mbed-ci
Copy link

mbed-ci commented Feb 28, 2018

Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving since it looks like this PR has addressed all outdated comments.

@cmonr cmonr merged commit 3d1174a into ARMmbed:master Feb 28, 2018
@deepikabhavnani deepikabhavnani deleted the common_crc branch February 28, 2018 18:07
Copy link
Contributor

@SenRamakri SenRamakri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

*/
template <uint32_t polynomial, uint8_t width>
MbedCRC<polynomial, width>::MbedCRC(uint32_t initial_xor, uint32_t final_xor, bool reflect_data, bool reflect_remainder):
_initial_value(initial_xor), _final_xor(final_xor), _reflect_data(reflect_data), _reflect_remainder(reflect_remainder), _crc_table(NULL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add line break before _crc_table to align with other declarations style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants